Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI Windows changes #320

Closed
wants to merge 13 commits into from

Conversation

rosslwheeler
Copy link
Contributor

@rosslwheeler rosslwheeler commented May 1, 2024

@Ricardicus This is the first phase - just want to check on the structure. I'll put some more comments over here #229.

Adding Windows build support into CI
@Ricardicus
Copy link
Contributor

Nice! I was wondering when this would come since you added Windows support 😊 I have been using nmake from VS on windows. Nice to see there is another way. Is it possible to build the cuda also? Maybe it takes too long to install for the CI?

@rosslwheeler
Copy link
Contributor Author

Yes, the CI is taking too long when I install Cuda for WIndows dynamically but am trimming down the packages installed to see if I can get the time down.

@Ricardicus
Copy link
Contributor

The time it took is 5m 26s and Ubuntu variant is slower at 5m 33s. They run in parallel so this does not add more time to the CI I think. I think this looks solid!

@Ricardicus
Copy link
Contributor

Ricardicus commented May 1, 2024

Since the cuda build requires installing cuda which takes a lot of time, I think this is a good start. I can imagine that one could have a different trigger for that workflow action like periodically. I don't know if one can do it periodically here, haven't tested that. Would be neat. Perhaps just start here in this PR and add cuda windows build in another pr.

@rosslwheeler
Copy link
Contributor Author

rosslwheeler commented May 1, 2024

Sound good! Makes sense to me. I'll do the follow-on PR once we get the Cuda timings down and/or have a solution periodically that seems reasonable.

@Ricardicus
Copy link
Contributor

Does it build with open MP support here in the windows CI?

@rosslwheeler
Copy link
Contributor Author

Yes, it's defaulting to OpenMP on. I didn't split out all the different versions (e.g. OpenMP off etc.) but can if we want to.

@Ricardicus
Copy link
Contributor

Ok! No I don't think it is needed. Just wondering. LGTM 👍

@rosslwheeler
Copy link
Contributor Author

Okay, this is ready to go. Reduced the Windows Cuda builds (Cuda installation mainly) time from 20+ minutes to 2 minutes.

image

Significant speed up in Windows Cuda build 20+ minutes to 2 minutes currenlty.

Add windows check for OpenMP install
@Ricardicus
Copy link
Contributor

Very cool! Great work. I see that you download and install the cuda dependencies directly. I wonder why the official installation is so much slower.

@rosslwheeler
Copy link
Contributor Author

Yes, not sure why the executable installer is so much slower.

Still looking into getting us some Cuda github runners too. Looks like we might need a team/enterprise GitHub account to get one (and that's not a guarantee either).

@rosslwheeler
Copy link
Contributor Author

Updated with this one #401 - it's ready to go.

@rosslwheeler rosslwheeler deleted the ci-windows-changes branch May 13, 2024 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants